Support hiding window title when using vertical tab strip #15523

sangwoo108 · 2022-10-19T00:04:17Z

Resolves brave/brave-browser#25473
Resolves brave/brave-browser#25757

Add a tab context menu to hide window title when vertical tab strip is enabled. When hiding window title, we can claim title area so that users can use taller contents area.

On Mac and Window, toolbar will take the title area.
On Linux, title area will be as short as possible
- can't overlay caption buttons on toolbar for now.

Also, we should make it easy to move window when hiding window title as there's almost no non-client area. Thus, enable dragging windows by dragging toolbar area.

Windows glass frame

no title

with title

Windows opaque frame

with title

no title

Mac

With title

no title

Linux

With title

No title

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Manual

When using vertical tab strip, there should be a context menu item to show/hide window title
When toggling window title state, window title visibility should be changed
When hiding window title, non client area should be at minimum size.
Dragging toolbar will moving the window

Automated

VerticalTabstripBrowserTest.WindowTitleVisibility.Toolbar
BraveNonClientHitTestHelperBrowserTest.WindowTitle

browser/ui/views/tabs/features.h

fallaciousreasoning · 2022-10-20T20:41:51Z

app/brave_generated_resources.grd

      </if>
      <if expr="use_titlecase">
        <message name="IDS_TAB_CXMENU_BOOKMARK_ALL_TABS" desc="In Title Case: The label of the tab context menu item for creating a bookmark folder containing an entry for each open tab.">
          Bookmark All Tabs...
        </message>
+        <message name="IDS_TAB_CXMENU_SHOW_TITLE_BAR" desc="The label of the tab context menu item for showing window title when vertical tab strip is enabled.">
+          Show Window Title


It's kinda annoying how we have to do this

Yeah, indeed. I wish we could automate casing

fallaciousreasoning

LGTM, might be worth trying to add some browser tests though. I'm excited for this feature!

Edit:

I just tried changing the theme to Use GTK - it crashes the latest nightly when vertical tabs are enabled. On your branch it does nothing (even when vertical tabs are disabled). Might be worth looking into.

Also, playing around with the setting on Linux it's quite hard to drag the window around now

And I think the window icons come a bit out of the frame

app/brave_generated_resources.grd

browser/ui/views/frame/brave_browser_frame_mac.mm

browser/ui/views/frame/brave_glass_browser_frame_view.cc

browser/ui/views/tabs/brave_tab_context_menu_contents.cc

browser/ui/views/tabs/features.cc

browser/ui/views/tabs/features.h

chromium_src/chrome/browser/ui/views/frame/browser_frame_view_layout_linux.cc

simonhong · 2022-10-21T03:01:11Z

On linux, I always enable system title and borders. Title is located in the center of title bar.
and that center position looks good for me. It's just a suggestion :)

browser/ui/views/frame/brave_browser_frame_mac.mm

browser/ui/views/toolbar/brave_toolbar_view.cc

sangwoo108 · 2022-10-21T07:25:40Z

@fallaciousreasoning

it crashes the latest nightly when vertical tabs are enabled

Err, it might be due to overlay mode. Let me check it out.

Also, playing around with the setting on Linux it's quite hard to drag the window around now

It seems HTCAPTION things are not working as expected on Linux. @simonhong Are you using linux too?

And I think the window icons come a bit out of the frame

This shouldn't happen :( The height looks incorrect when using GTK theme.

Also, playing around with the setting on Linux it's quite hard to drag the window around now

It seems your version uses caption button based on vector icon, which I didn't expect:( 18 seems to be too short for it. Let me dig this deeper. Anyway, I think dragging with toolbar area should work now.

sangwoo108 · 2022-10-21T07:26:53Z

@simonhong

On linux, I always enable system title and borders. Title is located in the center of title bar.

Yeah, I totally agree. I'm considering that too but window title implementation for each platform differs so I think I could do it as a follow-up.

sangwoo108 · 2022-10-23T07:22:53Z

might be worth trying to add some browser tests though

Now we have browser tests too and other things are addressed, I think. @simonhong @fallaciousreasoning Do you mind taking another look?

fallaciousreasoning

LGTM

fallaciousreasoning · 2022-10-23T20:42:06Z

chromium_src/ui/views/widget/native_widget_mac.h

+    return overridden_window_title_visibility_.has_value(); \
+  }                                                         \
+  bool overridden_window_title_visibility() const {         \
+    return overridden_window_title_visibility_.value();     \


Should we do a .value_or(false) here? It's not obvious that the method will crash if you don't check has_overridden_window_title_visibility first.

It's not obvious that the method will crash

I thought it'd crash, but it seems it's close to undefined behavior. I think it's better to let it crash to detect problems for some cases, and this is the case. And also I intentionally didn't use .value_or() here, because when there's no overridden visibility, we should use WidgetDelegate::ShouldShowWindowTitle() to know about the default visibility.

Now I find it that DCHECK(has_overriden_visibility()) << "Use WidgetDelegate::ShouldShowWindowTitle()" is more explicit and better, thanks to your comment.

chromium_src/ui/views/widget/native_widget_mac.h

fallaciousreasoning

LGTM

simonhong

++ 👍🏼

chromium_src/components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm

goodov

few nits + can you please double-check that new chromium overrides are fine without VerticalTabs enabled? to make sure we didn't break some edge case when the feature is off.

goodov · 2022-10-24T08:38:23Z

browser/ui/views/tabs/features.cc

+    if (const gfx::ImageSkia* image = tp->GetImageSkiaNamed(image_id))
+      width += image->width();
+  }
+  return {0, width};


are all of these numbers work correctly with different OS-level screen scale?

Yes, it looks so. Both seems to be DIP.

goodov · 2022-10-24T08:40:51Z

browser/ui/views/frame/brave_glass_browser_frame_view.cc

+  return GlassBrowserFrameView::GetTopInset(restored);
+}
+int BraveGlassBrowserFrameView::NonClientHitTest(const gfx::Point& point) {
+  if (auto res = brave::NonClientHitTest(browser_view(), point);


should this check VerticalTabs feature?

I think we'd better to support this for others too. brave/brave-browser#25757

sangwoo108

can you please double-check that new chromium overrides are fine without VerticalTabs enabled?

Yep sure. Confirmed that there's no crash and frame view works well without the flag.

I found that one of macro was undef-ed, fixed it and add a static check for it. PTAL thanks :)

sangwoo108 · 2022-10-24T09:51:50Z

browser/ui/views/frame/brave_glass_browser_frame_view.cc

+  return GlassBrowserFrameView::GetTopInset(restored);
+}
+int BraveGlassBrowserFrameView::NonClientHitTest(const gfx::Point& point) {
+  if (auto res = brave::NonClientHitTest(browser_view(), point);


I think we'd better to support this for others too. brave/brave-browser#25757

sangwoo108 · 2022-10-24T10:44:29Z

browser/ui/views/tabs/features.cc

+    if (const gfx::ImageSkia* image = tp->GetImageSkiaNamed(image_id))
+      width += image->width();
+  }
+  return {0, width};


Yes, it looks so. Both seems to be DIP.

Add a tab context menu to hide window title when vertical tab strip is enabled. When hiding window title, we can claim title area so that users can use taller contents area. * On Mac and Window, toolbar will take the title area. * On Linux, title area will be as short as possible * can't overlay caption buttons on toolbar for now. Also, we should make it easy to move window when hiding window title as there's almost no non-client area. Thus, enable dragging windows by dragging toolbar area.

When TabStrip::ShouldDrawColor() returns true, BrowserRootView will try to clip bounds tab strip. But When vertical tabstrip is enabled, converting coordinate system will fail as we have another widget for vertical tab strip. As we don't have to draw strokes below vertical tab strip, returning false will be okay for our purpose.

sangwoo108 · 2022-10-25T09:37:17Z

Thank you all for reviewing this :)

bridiver · 2023-01-03T20:32:36Z

browser/ui/views/tabs/features.cc

+  }
+  return {0, width};
+#else
+#error "not handled platform"


NOTREACHED()

github-actions bot assigned sangwoo108 Oct 19, 2022

github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Oct 19, 2022

sangwoo108 commented Oct 19, 2022

View reviewed changes

browser/ui/views/tabs/features.h Outdated Show resolved Hide resolved

sangwoo108 force-pushed the sko/vtab-toolbar branch from 6851d00 to 2858f52 Compare October 20, 2022 05:53

sangwoo108 marked this pull request as ready for review October 20, 2022 05:56

sangwoo108 requested a review from a team as a code owner October 20, 2022 05:56

sangwoo108 force-pushed the sko/vtab-toolbar branch from 2858f52 to 975dc2f Compare October 20, 2022 05:56

sangwoo108 requested review from simonhong, fallaciousreasoning and petemill October 20, 2022 06:00

sangwoo108 force-pushed the sko/vtab-toolbar branch 5 times, most recently from 5ad9948 to 74512c2 Compare October 20, 2022 14:49

fallaciousreasoning reviewed Oct 20, 2022

View reviewed changes

fallaciousreasoning approved these changes Oct 20, 2022

View reviewed changes

simonhong reviewed Oct 21, 2022

View reviewed changes

browser/ui/views/frame/brave_browser_frame_mac.mm Show resolved Hide resolved

browser/ui/views/toolbar/brave_toolbar_view.cc Show resolved Hide resolved

sangwoo108 force-pushed the sko/vtab-toolbar branch 7 times, most recently from 98bcbd7 to 5336bd8 Compare October 23, 2022 07:15

fallaciousreasoning approved these changes Oct 23, 2022

View reviewed changes

fallaciousreasoning reviewed Oct 23, 2022

View reviewed changes

sangwoo108 force-pushed the sko/vtab-toolbar branch from 96575b3 to 9fdcb57 Compare October 23, 2022 23:51

simonhong approved these changes Oct 24, 2022

View reviewed changes

chromium_src/components/remote_cocoa/app_shim/native_widget_ns_window_bridge.mm Show resolved Hide resolved

goodov reviewed Oct 24, 2022

View reviewed changes

sangwoo108 commented Oct 24, 2022

View reviewed changes

sangwoo108 force-pushed the sko/vtab-toolbar branch from 5675d49 to 50ce5ed Compare October 24, 2022 12:55

sangwoo108 added 11 commits October 25, 2022 11:45

Make frame views on linux use BraveOpaqueBrowserFrameView

2448aa4

Clean up

2ecad19

Make tests for title visibility and hit test

0e8b9a6

Update BrowserFrameViewLayoutLinux::NonClientTopHeight

08f1c16

Add DCHECK

059a8c7

Fix test failure

2ff2d39

Fix include bug

853171f

Fix test failure on Windows

b0577ba

sangwoo108 force-pushed the sko/vtab-toolbar branch from d02e2c8 to 22e94a4 Compare October 25, 2022 03:02

goodov approved these changes Oct 25, 2022

View reviewed changes

Fix linux test case

8ec1ab6

sangwoo108 force-pushed the sko/vtab-toolbar branch from 22e94a4 to 8ec1ab6 Compare October 25, 2022 06:56

sangwoo108 merged commit 0645d2a into master Oct 25, 2022

sangwoo108 deleted the sko/vtab-toolbar branch October 25, 2022 09:37

github-actions bot added this to the 1.47.x - Nightly milestone Oct 25, 2022

sangwoo108 mentioned this pull request Oct 25, 2022

Polish vertical tab UI for the initial version brave/brave-browser#24717

Closed

26 tasks

bridiver reviewed Jan 3, 2023

View reviewed changes

browser/ui/views/tabs/features.cc

}

return {0, width};

#else

#error "not handled platform"

Copy link

Collaborator

bridiver Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTREACHED()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hiding window title when using vertical tab strip #15523

Support hiding window title when using vertical tab strip #15523

sangwoo108 commented Oct 19, 2022 •

edited

Loading

fallaciousreasoning Oct 20, 2022

sangwoo108 Oct 21, 2022

fallaciousreasoning left a comment •

edited

Loading

simonhong commented Oct 21, 2022

sangwoo108 commented Oct 21, 2022 •

edited

Loading

sangwoo108 commented Oct 21, 2022 •

edited

Loading

sangwoo108 commented Oct 23, 2022

fallaciousreasoning left a comment

fallaciousreasoning Oct 23, 2022

sangwoo108 Oct 23, 2022

fallaciousreasoning left a comment

simonhong left a comment

goodov left a comment •

edited

Loading

goodov Oct 24, 2022

sangwoo108 Oct 24, 2022

goodov Oct 24, 2022

sangwoo108 Oct 24, 2022

sangwoo108 left a comment

sangwoo108 Oct 24, 2022

sangwoo108 Oct 24, 2022

sangwoo108 commented Oct 25, 2022

bridiver Jan 3, 2023

Support hiding window title when using vertical tab strip #15523

Support hiding window title when using vertical tab strip #15523

Conversation

sangwoo108 commented Oct 19, 2022 • edited Loading

Windows glass frame

no title

with title

Windows opaque frame

with title

no title

Mac

With title

no title

Linux

With title

No title

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Manual

Automated

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fallaciousreasoning left a comment • edited Loading

Choose a reason for hiding this comment

simonhong commented Oct 21, 2022

sangwoo108 commented Oct 21, 2022 • edited Loading

sangwoo108 commented Oct 21, 2022 • edited Loading

sangwoo108 commented Oct 23, 2022

fallaciousreasoning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

goodov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangwoo108 commented Oct 25, 2022

Choose a reason for hiding this comment

sangwoo108 commented Oct 19, 2022 •

edited

Loading

fallaciousreasoning left a comment •

edited

Loading

sangwoo108 commented Oct 21, 2022 •

edited

Loading

sangwoo108 commented Oct 21, 2022 •

edited

Loading

goodov left a comment •

edited

Loading